-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the same executor for pyiron table and function container #1334
Conversation
Merge main
# Conflicts: # .ci_support/environment.yml # setup.py
…_executor # Conflicts: # pyiron_base/jobs/job/generic.py
@liamhuber Thanks for your review, I tried to address the individual points in the separate comments. I just do not feel so easy to allow all possible executors, I prefer to whitelist existing executors and when somebody wants to use specific executors with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One blocking comment re: turning the functionality back off and one minor comment re: UX.
Otherwise, I won't insist on these but I would encourage you to carefully consider this feedback, which is two closely related ideas:
I just do not feel so easy to allow all possible executors, I prefer to whitelist existing executors and when somebody wants to use specific executors with
pyiron_base
and adds the corresponding tests I am happy to add those to the white list.
(Non-blocking) Walled-garden
By adhering to a strict whitelist paradigm, you force new users to adopt the pyiron ecosystem entirely, or avoid pyiron. If a user comes along with some new executor they want to use, they need to explicitly modify pyiron_base
just to check if it works.
In contrast, if we place some reasonable constraint on the value (i.e. inherits from the base concurrent.futures.Executor
to guarantee basic interface), and give guidance that certain only executors are guaranteed to work, then users can experiment with new things and pull pyiron into their preferred toolset. Then, if it works, they are free to suggest that their executor be added to the test suite, etc.
IMO this is the much more pythonic approach, and avoids locking-out users with particular executor demands.
Apart from this I have an intrinsic motivation to promote the use of
pympipool
in particular...
(Non-blocking) Nudge don't force
This is an extremely fair perspective. I would encourage nudging users instead of forcing them to adopt a particular behaviour though. In this case, the pympipool
executors should be "promoted" and "supported", so we mention them by name, have tests to guarantee they work, and lower the barrier to using them relative other executors.
This is exactly what I do over in pyiron_workflow
, where Node.executor
must inherit from the base executor to guarantee interface, warnings are made that executors need to support serializing dynamic stuff with docs directing users to pympipool
, and then the pympipool
executors are made as accessible as possible by storing them in Workflow.create.executor
.
I would suggest a similar architecture here, so that pympipool
holds a place of extreme privilege without being exclusive.
@liamhuber I followed your suggestion and added the option to define executors as both strings and python classes. From my perspective this pull request is now ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I recommend extending the tests to be a bit more thorough for the three executor cases.
Combine the work in #1050 and #1296
Example code - for parallel pyiron table:
EDIT: syntax highlight flag